Skip to content

Aliases for 2D and 3D Float64 vectors. #655

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Conversation

EvertSchippers
Copy link
Collaborator

To simplify working with 3D data, an easy and familiar alias for 2D and 3D vectors.

@tkoolen
Copy link
Contributor

tkoolen commented Sep 11, 2019

In my opinion, this is not a good idea. What makes Float64 special? Vectors of Float32 are also quite common, with GPU-related packages probably using them exclusively. If these aliases make sense in the context of a certain package, they can be defined there with no performance cost. Also, this promotes writing code that is not type-generic.

And then there's #24 (comment), #24 (comment), and #24 (comment).

@c42f
Copy link
Member

c42f commented Sep 11, 2019

Hi, thanks for the PR!

However I don't think we should privilege Float64 like this in StaticArrays itself, especially not without a systematic naming scheme which would allow short names for other common eltypes like Float32. Basically what @tkoolen has already said.

If we did want short forms, some naming inspiration could come from Base.ComplexF64 and FixedPointNumbers.N0f8, N0f16 etc. Eg it could be useful to have V3F64 and V3F32. I'm not sure we need these though; there seems to be two use cases:

  1. Really convenient constructors — these can largely be served by the proposed SA syntax from WIP/RFC: Implement SA[...] syntax for SArray literals #633
  2. Type declarations within user-defined structs — I feel that the burden of writing out the full type SVector{3,Float64} is not a problem here. (SMatrix is admittedly not so great and would ideally be fixed in Base.)

Perhaps you can describe your use case, especially if it's not covered by the above?

@EvertSchippers
Copy link
Collaborator Author

Twan, I think you're right about the Float64 vs Float32. Currently we do declare them in our own packages. It's just that we declare it again and again. At some point, someone will indeed decide to make Vector3D in another package a Float32 vector and then suddenly Vector3D means different things in different packages... So it seemed nice to declare it in a common place. Better to do just that but in another shared module.
And somehow the build fails!! So let's not merge :)
How about a tiny submodule inside StaticArrays, so besides "using StaticArrays" one could also specify "using StaticArrays.Float64Vectors" and thereby importing these Float64 variants or Vector3D and Vector2D?

@tkoolen
Copy link
Contributor

tkoolen commented Sep 11, 2019

using StaticArrays.Float64Vectors

This is an interesting idea that I haven't seen discussed before. But I think a flaw is that the natural follow-up PR would be to add Float32Vectors, and then you run into the same issue you pointed out:

At some point, someone will indeed decide to make Vector3D in another package a Float32 vector and then suddenly Vector3D means different things in different packages...

Also, you'd still be repeating using StaticArrays.Float64Vectors in every package; that's not a huge win over just typing one or two type aliases.

@c42f brings up good points. What's the main quality-of-life improvement you're looking for?

By the way, in Eigen, V3F64 and V3F32 are spelled Vector3d (d for double, not dimension) and Vector3f (for float) respectively. Unfortunately, double and float are not the terminology used in Julia, so it doesn't make that much sense to copy those names.

@EvertSchippers
Copy link
Collaborator Author

Yes, you'd be repeating the using, but you'd won't override Vector3D (with the same thing, but new line of code) when using two packages that both define Vector3D. And there would be no code duplication in case something would ever change about Vector3D.

@EvertSchippers
Copy link
Collaborator Author

W.r.t the quality of life, Vector3D is simply more readable than SVector{3, Float64}. A list of points would be Vector{Vector3D}, which is clear, but Vector{SVector{3,Float64}} already becomes distracting when you want to quickly see what code does...

@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.694% when pulling c792afb on vectoralias into c607c0d on master.

@c42f
Copy link
Member

c42f commented Sep 11, 2019

Vector3D is simply more readable than SVector{3, Float64}.

I certainly agree with that. But can you describe the specific circumstances in which you typically need to write SVector{3, Float64}? Even just paste in specific code examples?

By the way, I definitely don't think we should have StaticArrays.Float64Vectors and StaticArrays.Float32Vectors both define Vector3D — that just means nobody can tell which variety they're using by eyeballing the code without reading the using statements.

@EvertSchippers
Copy link
Collaborator Author

Just exploring options! I also feel there's something fishy about that.
I get the feeling it's best to keep it in house after all.
Thanks for thinking along guys, much appreciated! I'll have a chat here tomorrow and most likely just close this PR...

@c42f
Copy link
Member

c42f commented Sep 11, 2019

For sure, I'm happy to have the discussion.

To be clear, I highly value usability and I'd like to merge a PR which makes things easier for you while respecting the generic nature of the library. It's just that resolving those two things together is difficult because our types are highly parametric in both shape and eltype so it's hard to be consistent and also avoid massive namespace pollution.

An option is to simply define a submodule StaticArrays.Aliases which exports a bunch of type aliases for "the usual suspects"

julia> module Aliases
       using StaticArrays

       for (T,tn) in [(Float32, "F32"), (Float64, "F64")]
           for L in [2,3,4]
               name = Symbol("Vec$L$tn")
               @eval const $name = SVector{$L, $T}
               mname = Symbol("Mat$L$L$tn")
               @eval const $mname = SMatrix{$L, $L, $T}
               @eval export $name, $mname
           end
       end

       end
WARNING: replacing module Aliases.
Main.Aliases

julia> names(Aliases)
13-element Array{Symbol,1}:
 :Aliases 
 :Mat22F32
 :Mat22F64
 :Mat33F32
 :Mat33F64
 :Mat44F32
 :Mat44F64
 :Vec2F32 
 :Vec2F64 
 :Vec3F32 
 :Vec3F64 
 :Vec4F32 
 :Vec4F64 

Eh, I'm not sure I find this terribly compelling though.

Another option would be to define some type aliases for eltypes of Float32 and Float64, for example

const SVectorF32{N} = SVector{N,Float32}
const SMatrixF64{N,M} = SMatrix{N,M,Float64}

This might be more useful because having a name for commonly used eltypes is more managable than having a big pile of names for each dimensionality. (Unfortunately SMatrixF64{N,M} is an abstract type right now and fixing that would require some difficult surgery to Base. Beyond that, the bare constructor for that type is just not that useful as it requires unnatural column major entry of elements.)

@c42f
Copy link
Member

c42f commented Sep 14, 2019

I think a reasonable compromise solution here is to use #633 and to add some type aliases:

SA_F64 = SA{Float64}
SA_F32 = SA{Float32}

v = SA_F64[1,2,3]  # A Float64 vector

v = SA_F32[1 2; 3 4]  # A 2x2 Float32 SMatrix

I don't love the underscore there, but SAF64 is a lot of capitals.

This solves the issue of constructing these types with a precise eltype, but without needing a verbose type name. I suspect this is the major quality of life improvement people really want here (correct me if I'm wrong).

@c42f
Copy link
Member

c42f commented Sep 14, 2019

I've implemented the SA_F64 alias in #633 and I think it will finally solve the long standing problem of constructors. Thanks for stimulating the discussion on this once again!

I'm going to close this PR, as I'm not sure what further action we can take here. But please do continue the discussion, especially if you have some motivating use cases which can be clearly described.

@c42f c42f closed this Sep 14, 2019
@c42f c42f added the feature features and feature requests label Sep 14, 2019
@EvertSchippers
Copy link
Collaborator Author

Thanks again guys!! A good step I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature features and feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants